@noaov1 reviewed 6 files and all commit messages, and made 4 comments.
Reviewable status: 6 of 8 files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @AvivYossef-starkware).

crates/starknet_os_runner/src/virtual_block_executor.rs line 21 at r3 (raw file):

/// Captures execution data for a virtual block (multiple transactions).
///
/// A virtual block is a set of transactions executed together without block preprocessing,

?

Code quote:

block preprocessing,

crates/starknet_os_runner/src/virtual_block_executor.rs line 23 at r3 (raw file):

/// A virtual block is a set of transactions executed together without block preprocessing,
/// useful for OS input generation and proving. This struct contains all the execution
/// outputs, block context, and initial state reads needed for proof generation.

Suggestion:

/// useful for OS input generation and proving. This struct contains all the transaction execution
/// outputs, block context, and initial state reads needed for proof generation.

crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

/// # Note
///
/// Currently only Invoke transactions are supported.

Why does it matter?

Code quote:

/// Currently only Invoke transactions are supported.

crates/starknet_os_runner/src/virtual_block_executor.rs line 59 at r3 (raw file):

/// ```
pub trait VirtualBlockExecutor {
    /// Executes a virtual block at the given block number.

Suggestion:

    /// Executes a virtual block based on the state abd context of the given block number.

@AvivYossef-starkware made 2 comments and resolved 2 discussions.
Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @noaov1).

crates/starknet_os_runner/src/virtual_block_executor.rs line 21 at r3 (raw file):

here

crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

The way I convert it to a blockifier transaction.

@noaov1 reviewed 2 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @avi-starkware and @AvivYossef-starkware).

crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

Yes, I saw. Why do you enforce it?
Why not use api_txs_to_blockifier_txs_next_block (and rename it)?

crates/starknet_os_runner/src/virtual_block_executor.rs line 26 at r5 (raw file):

pub struct VirtualBlockExecutionData {
    /// Execution outputs for all transactions in the virtual block.
    pub execution_outputs: Vec<TransactionExecutionOutput>,

Why do we need the state maps?

Code quote:

TransactionExecutionOutput

crates/starknet_os_runner/src/virtual_block_executor.rs line 45 at r5 (raw file):

///
/// - Currently only Invoke transactions are supported.
/// - Validation and fee charging are always skipped (useful for simulation/proving).

What do you mean by validations? __validate__?

Code quote:

/// - Validation and fee charging are always skipped (useful for simulation/proving).

crates/starknet_os_runner/src/virtual_block_executor.rs line 97 at r5 (raw file):

            validate: true,
            charge_fee: false,
            strict_nonce_check: false,

Why?

Code quote:

            strict_nonce_check: false,

crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 205 at r5 (raw file):

    /// Creates an RpcStateReader from a node URL, chain ID, and block number.
    pub fn new_with_default_config(

Suggestion:

pub fn new_with_config_from_url(

@AvivYossef-starkware made 4 comments and resolved 1 discussion.
Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware).

crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

it would fail in declare tx here
I wanted to clarify this.

crates/starknet_os_runner/src/virtual_block_executor.rs line 26 at r5 (raw file):

should I verify that the state diff is empty?

crates/starknet_os_runner/src/virtual_block_executor.rs line 45 at r5 (raw file):

Done

crates/starknet_os_runner/src/virtual_block_executor.rs line 97 at r5 (raw file):

@Yoni-Starkware

@noaov1 reviewed 2 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @AvivYossef-starkware, and @Yoni-Starkware).

crates/starknet_os_runner/src/virtual_block_executor.rs line 44 at r3 (raw file):

I see

crates/starknet_os_runner/src/virtual_block_executor.rs line 26 at r5 (raw file):

I thought that we will verify it in the os itself.

@noaov1 made 1 comment and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @AvivYossef-starkware, and @Yoni-Starkware).

crates/starknet_os_runner/src/virtual_block_executor.rs line 97 at r5 (raw file):

Oh, I see, we don't want to enforce strict nonce as the state of the account does not matter

@noaov1 made 1 comment and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware and @Yoni-Starkware).

@Yoni-Starkware made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved.

crates/starknet_os_runner/src/virtual_block_executor.rs line 97 at r5 (raw file):

@AvivYossef-starkware, you should set the charge_fee flag the same as the blockifier. Currently, the OS skips the fee charging only if the resource bounds are "zero"

@Yoni-Starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion.

crates/starknet_os_runner/src/virtual_block_executor.rs line 205 at r6 (raw file):

        Ok(VirtualBlockExecutionData { execution_outputs, block_context, initial_reads })
    }

Most of this code is general. Since you defined a trait for that, please move it to the trait, and extract out the necessary parts (like state reader fetch)

Code quote:

    fn execute_inner(
        &self,
        block_number: BlockNumber,
        txs: Vec<BlockifierTransaction>,
    ) -> Result<VirtualBlockExecutionData, VirtualBlockExecutorError> {
        // Create RPC state reader for the given block.
        let rpc_state_reader = RpcStateReader::new_with_config_from_url(
            self.node_url.clone(),
            self.chain_id.clone(),
            block_number,
        );

        // Get block context from RPC.
        let block_context = rpc_state_reader
            .get_block_context()
            .map_err(|e| VirtualBlockExecutorError::ReexecutionError(Box::new(e)))?;

        // Create state reader with contract manager.
        let state_reader_and_contract_manager = StateReaderAndContractManager::new(
            rpc_state_reader,
            self.contract_class_manager.clone(),
            None,
        );

        let block_state = CachedState::new(state_reader_and_contract_manager);

        // Create executor WITHOUT preprocessing (no pre_process_block call).
        let mut transaction_executor = TransactionExecutor::new(
            block_state,
            block_context.clone(),
            TransactionExecutorConfig::default(),
        );

        // Execute all transactions.
        let execution_results = transaction_executor.execute_txs(&txs, None);

        // Collect results, returning error if any transaction fails.
        let execution_outputs: Vec<TransactionExecutionOutput> = execution_results
            .into_iter()
            .map(|result| {
                result.map_err(|e| {
                    VirtualBlockExecutorError::TransactionExecutionError(e.to_string())
                })
            })
            .collect::<Result<Vec<_>, _>>()?;

        // Get initial state reads.
        let initial_reads = transaction_executor
            .block_state
            .as_ref()
            .ok_or(VirtualBlockExecutorError::StateUnavailable)?
            .get_initial_reads()
            .map_err(|e| VirtualBlockExecutorError::ReexecutionError(Box::new(e.into())))?;

        Ok(VirtualBlockExecutionData { execution_outputs, block_context, initial_reads })
    }

@Yoni-Starkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions.

crates/starknet_os_runner/src/virtual_block_executor.rs line 37 at r6 (raw file):

/// A virtual block executor runs transactions without block preprocessing
/// (`pre_process_block`), which is useful for simulating execution or generating
/// OS input for proving.

Suggestion:

/// A virtual block executor runs transactions on top of a given, finalized block. This means that some parts, like block preprocessing
/// (`pre_process_block`), are skipped. Useful for simulating execution or generating
/// OS input for proving.